Refactor/properties locale and paste constant#32
Conversation
The literal 20 was repeated three times across duplicateObject, duplicateSelectedObjects, and pasteObjects as the per-copy positional offset. Name it once with a comment that explains the dots/mm relation so future tweaks happen in one place.
PropertiesPanel rendered '{n} objects selected' and 'use arrow keys to
move' as hardcoded English in an otherwise fully-localised UI. Add two
keys (multipleSelectedFmt, multipleSelectedHint) under properties; the
former carries an {n} placeholder substituted at render time. All 32
locales filled in via add_locale_key.local.py.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements localization for the properties panel selection text across various languages and introduces a DUPLICATE_OFFSET_DOTS constant to replace hardcoded values in the label store. Feedback suggests refining the documentation for the new constant and addressing a logic bug in duplicateSelectedObjects that results in exponential staggering of duplicated items.
Gemini caught a bug surfaced by the constant extraction: the previous 'duplicateCount * DUPLICATE_OFFSET_DOTS' produced quadratic stagger (positions 20, 60, 120, 200…) because the selection moves to each new copy, then the next call multiplies by an ever-growing duplicateCount on top of that already-shifted source. The selection-follows-copy mechanic already produces linear stagger on its own — the multiplier was unwarranted. Use a constant offset. duplicateCount state and its selection-reset bookkeeping become dead; remove them. pasteObjects keeps its multiplier: the clipboard source is static, so multiplication is the only thing producing stagger. Add regression tests for duplicateSelectedObjects (none existed, which is how the bug shipped). Update the constant docstring to reflect the now-correct two-mode behaviour.
duplicateObject and duplicateSelectedObjects had near-identical bodies after the previous fix unified them on a constant offset. Pull the shared logic (find by id, spread + new id + offset, drop missing) into a single helper so both actions reduce to selection-shape concerns.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the object duplication logic by introducing a buildOffsetCopies helper function and a DUPLICATE_OFFSET_DOTS constant, while removing the redundant duplicateCount state. It also updates the UI to support localized strings for multiple selection and adds unit tests for the new duplication behavior. A review comment suggested improving buildOffsetCopies by performing a deep clone of the props object to prevent shared state mutations and using a Map for better lookup performance.
Two improvements from a Gemini review: - Shallow-clone props on copy. Matches the copySelectedObjects pattern. Today nothing mutates props directly (updateObject builds a fresh object via Object.assign), so the shared reference is invisible — but cheap to defend now rather than wait for a future contributor's in-place edit to leak across copies. - Index objs in a Map before the flatMap pass. The previous nested find was O(N×M); for typical labels both are tiny so it's not a real performance issue, but the Map form reads as 'looking things up by id' rather than 'scanning each time'.
1 participant
No description provided.